-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert using a separate draw thread #4656
Conversation
internal/animation/runner.go
Outdated
} | ||
} | ||
r.animationMutex.Lock() | ||
r.animations = append(newList, r.pendingAnimations...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The runner can get a whole lot cleaner once we assume the new threading model. While Start and Stop can still be called when invoking user code within a Tick, we will know they won't be called elsewhere, so we can do some simplification and cleanup around the synchronization between Start, Stop and Tick. That can be a follow-up PR though.
Putting this on hold until we begin work on 2.6 |
ecb0c1f
to
3723d6e
Compare
Reworked on top of This seems to work smoothly in limited testing but we need to get it checked on all OS. |
P.S. in a follow up PR where I merge the event thread in as well I have hello and fyne_demo completely without race errors! Keeping them separate as this PR works smoothly but the event management change has significant performance impact! |
All Mac tests passing. There is a performance impact but it is actually clear where some latency is coming from - hovering events are triggering multiple event updates etc. This change may help with hunting down other performance bugs! |
Nice. That should help us decrease power consumption and unnecessary cpu usage :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done. Performance seems okay and everything appears to be working here on Linux. Just left one minor comment line but I'll approve before hand :)
drawFuncQueue <- drawData{f: f, win: w, done: done} | ||
<-done | ||
func runOnMainWithContext(w *window, f func()) { | ||
runOnMain(func() { w.RunWithContext(f) }) // TODO remove this completely |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a TODO for an upcoming PR or should it be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is part of the future refactoring. Lots more to be done as mentioned above and in the other tickets.
There are 3 different ways to queue onto what is essentially the same thread but I didn't want to rip out too much code until the direction is established :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Just wanted to check. Still approved from my end :)
Progresses #4654
Checklist:
Notice that on macOS the UI will stop drawing if we resize. This is the reason draw thread was added and we will need to find another solution...